Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes #1332

MohamedAbdeen21 · 2023-09-14T15:23:05Z

This PR addresses issue #1321, which is caused by the hard-coded value of MaximumTaskCall. This change adds the option to set number of max task calls before termination through the new --max-runs flag.

Example Taskfile

version: '3'

vars:
  RANGE: '0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120'

tasks:
  loop-over-range:
    vars:
      range: '{{.RANGE}}'
    cmds:
      - for:
          var: range
          split: ','
        vars:
          NUMBER: '{{.ITEM}}'
        task: echo-task

  echo-task:
    cmds:
      - echo '{{.NUMBER}}'

Old behavior

$ task loop-over-range
task: [echo-task] echo '0'
0
task: [echo-task] echo '1'
1
# ...
task: [echo-task] echo '98'
98
task: Failed to run task "loop-over-range": task: Maximum task call exceeded (0) for task "echo-task": probably an cyclic dep or infinite loop
exit status 201

Issue

Notice that in the old behavior:

The task runs for 99 (0 to 98) times instead of 100
The error message says that the maximum task call is 0.
There is no way to set the maximum number of task calls.

New behavior

$ task loop-over-range --max-runs 121
task: [echo-task] echo '0'
0
task: [echo-task] echo '1'
1
# ...
task: [echo-task] echo '119'
119
task: [echo-task] echo '120'
120

Added

The max-runs flag:
- Raises an error if set to less than 1
- If not set, uses task.MaximumTaskCall constant as default value

Fixed

Maximum Task Call Error
- Uses the value of max-runs correctly in the error message.
- Raised when the task is called exactly max-run times.

pd93 · 2023-09-14T19:01:02Z

I'm fine with this as a temporary solution if people are running into this issue a lot. However, the real problem is a bit more fundamental than this. MaximumTaskCall exists as a very arbitrary way of detecting cycles. However, all we're really doing is making an assumption that if you're calling a task many times, you're more likely than not to have a loop. It's not real cycle detection and there is no good value for this variable that will work for everyone.

This problem has been made much worse since we added loops via the for syntax as its now very easy to reach this arbitrary limit by looping over a task many times. The proper solution to this is real cycle detection. Probably via a task DAG.

We're actually very close to adding a Taskfile DAG and the Task DAG will follow on from that and will fix this issue outright (there will be no limit to how many times you can call a Task). It's up to @andreynering if we want to add this in the meantime. If this were to happen, it would probably be deprecated in the future.

MohamedAbdeen21 · 2023-09-14T19:34:27Z

Hey @pd93. While I totally agree with you, I think this feature can still be used even when the DAG is introduced; although the purpose would be slightly different. Instead of it being used as a way of detecting cycles/loops, it'll act similar to a timeout.

In other DAG tools, I've seen people writing code that iterates over a node (task, in this case) hundreds of times, through error retries or just normal looping, effectively wasting resources, and I've always appreciated having a timeout to free resources for other DAGs to run.

However, if you guys already have that case covered in the DAG implementation, then feel free to close the PR. Thank you for the amazing work.

pd93 · 2023-09-14T19:38:20Z

Understood, that makes sense. Perhaps once the DAG refactor happens, we can just remove the arbitrary limit of 100 (so it would be unlimited by default), and this setting can override that if a user wants to impose a manual limit. In that case, I'm happy to leave this open for review.

docs/docs/changelog.md

CHANGELOG.md

package.json

pd93 · 2023-09-14T19:47:32Z

cmd/task/task.go

@@ -135,6 +136,7 @@ func run() error {
 	pflag.DurationVarP(&flags.interval, "interval", "I", 0, "Interval to watch for changes.")
 	pflag.BoolVarP(&flags.global, "global", "g", false, "Runs global Taskfile, from $HOME/{T,t}askfile.{yml,yaml}.")
 	pflag.BoolVar(&flags.experiments, "experiments", false, "Lists all the available experiments and whether or not they are enabled.")
+	pflag.IntVar(&flags.maxRuns, "max-runs", task.MaximumTaskCall, "Set the maximum number of runs per task before being considered infinte or cyclic and therefore terminated.")


Not requesting any changes to this now. Just opening this up for thoughts and opinions as naming is always hard. Some other possibilities:

--limit - Nice and short, but might be too generic (could clash with something like include-limit if we wanted it)

--call-limit

--max-task-calls - Very explicit, but longer. I don't think length is a huge issue with these flags as they're most likely to be used in CI or scripts.

Also, I'd like to shorten the description. I couldn't find more concise (yet complete) description.

andreynering · 2023-10-07T21:31:21Z

Hi @MohamedAbdeen21,

I decided to just raise the limit as a mitigation for now. As @pd93 said, we'll hopefully have a much better detection soon, and then this arbitrary check will be gone.

a70f5aa

MohamedAbdeen21 added 4 commits September 14, 2023 17:52

Adding max-runs flag

3691845

Changed changelogs.md

b5db153

Cleaned tests

6bddb6a

version 3.31.0

52589b7

pd93 reviewed Sep 14, 2023

View reviewed changes

docs/docs/changelog.md Outdated Show resolved Hide resolved

pd93 reviewed Sep 14, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pd93 reviewed Sep 14, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

pd93 reviewed Sep 14, 2023

View reviewed changes

MohamedAbdeen21 added 2 commits September 14, 2023 22:52

Updating documention from review

6d48d50

Update changelog

58f7759

andreynering closed this in a70f5aa Oct 7, 2023

andreynering mentioned this pull request Oct 7, 2023

Maximum task call exceeded when looping over tasks #1321

Closed

pd93 mentioned this pull request Mar 25, 2024

feat: directed acyclic graph (DAG) #1563

Merged

pd93 mentioned this pull request Nov 29, 2024

"probably an cyclic dep or infinite loop" on huge graphs #820

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes #1332

Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes #1332

MohamedAbdeen21 commented Sep 14, 2023 •

edited

Loading

pd93 commented Sep 14, 2023

MohamedAbdeen21 commented Sep 14, 2023 •

edited

Loading

pd93 commented Sep 14, 2023

pd93 Sep 14, 2023 •

edited

Loading

MohamedAbdeen21 Sep 14, 2023

andreynering commented Oct 7, 2023

Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes #1332

Adding --max-runs flag to control number of times a task should run before termination + minor bug fixes #1332

Conversation

MohamedAbdeen21 commented Sep 14, 2023 • edited Loading

Example Taskfile

Old behavior

Issue

New behavior

Added

Fixed

pd93 commented Sep 14, 2023

MohamedAbdeen21 commented Sep 14, 2023 • edited Loading

pd93 commented Sep 14, 2023

pd93 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

MohamedAbdeen21 Sep 14, 2023

Choose a reason for hiding this comment

andreynering commented Oct 7, 2023

MohamedAbdeen21 commented Sep 14, 2023 •

edited

Loading

MohamedAbdeen21 commented Sep 14, 2023 •

edited

Loading

pd93 Sep 14, 2023 •

edited

Loading